Re-review: PR #26 — /recce-analyze round-2 fixes (0997ec9)

HEAD reviewed: 0997ec9
Mode: Incremental — re-review after round-2 fixes for the prior review at fb7146c.

Reviewed as code, not docs — the .md files are executable procedure for Claude Code and Codex.

Status of prior findings

The substantive fixes in both procedure files are correct. The trap-scope explanation prepended at recce-analyze.md:96-103 and AGENTS.md:80-85 is excellent — explicit, cites recce-setup.md:212-213, names the failure mode.

Verdict: NO-GO — 0 BLOCKERs, 1 ISSUE, 1 NOTE

The procedure fix is correct. The blocker here is narrower: the regression test for ISSUE 1 only catches the recce-analyze.md variant, not the AGENTS.md variant that the round-2 review explicitly named. The fix is ~3 lines and the commit message claims a guarantee the test doesn't actually deliver, so it's worth tightening before merge rather than deferring.

🟡 ISSUE

1. test_build_inside_trap_scope does not catch the AGENTS.md variant of round-2 ISSUE 1 — tests/test_merged_bootstrap.py:275-285

The new test asserts:

assert (
    "dbt docs generate" in body
    or "dbt build" in body
    or "<chosen build command>" in body
), ...

Substring-in-body matches comment text. I verified the pre-fix file states empirically with git show fb7146c:...:

The pre-fix AGENTS.md fence body is exactly the round-2 ISSUE 1 defect state for AGENTS.md, and this test passes against it.

The commit message says: "test_build_inside_trap_scope: scans every fenced bash block in each file, locates the one containing trap cleanup EXIT, and asserts a dbt command (or the <chosen build command> placeholder) appears in the same fence. Catches the exact ISSUE 1 regression." It catches the recce-analyze.md form. It does not catch the AGENTS.md form (comments-inside-fence-with-no-executable-line) — the very form the round-2 review explicitly flagged as a "sibling defect."

A future edit that removes the executable <chosen build command> line at AGENTS.md:108 and leaves only the documentation comments at AGENTS.md:106-107 regresses AGENTS.md back to the round-2 ISSUE 1 state while CI stays green.

Severity reasoning (I ran a structured Devil's Advocate debate on whether this should be NO-GO or GO-with-follow-up; the strongest contrarian argument is captured below):

• Mitigation: A future regression along this exact path would still fail loudly at runtime — missing target-base/manifest.json would produce an explicit error from recce check-base or the MCP diff tools on first execution. Not silent-wrong-answer. This lowers severity from BLOCKER-grade.
• But: the fix is ~3 lines, the commit message ships a stronger claim than the test delivers, and AGENTS.md is the more-likely-to-be-pattern-matched ingestion surface for Codex agents (where the test gap concentrates). Filing a follow-up risks the gap persisting indefinitely.

So: tighten the test in this PR rather than defer.

Fix. Filter to executable lines (drop comments and blanks) before the substring check:

def _exec_lines(body: str) -> str:
    return "\n".join(
        line for line in body.splitlines()
        if line.strip() and not line.strip().startswith("#")
    )

# inside the loop:
exec_text = _exec_lines(body)
assert (
    "dbt docs generate" in exec_text
    or "dbt build" in exec_text
    or "<chosen build command>" in exec_text
), (
    f"{label}: trap-protected fence must contain an EXECUTABLE dbt "
    "command or the `<chosen build command>` placeholder. Comments "
    "documenting which command to run do not satisfy — Codex/Claude "
    "running the block verbatim would no-op."
)

To verify the tightened test would actually catch the regression, mentally re-run it against the pre-fix AGENTS.md fence body shown above: only the git stash push … and git checkout <base-branch> lines survive the comment filter; none contain a dbt command or the placeholder; the assertion fails. ✅

Optional but recommended: add a regression test that constructs the pre-fix AGENTS.md fence body inline and asserts the tightened test fails on it — empirical proof the tightening covers the variant currently slipping through.

📘 NOTE

1. PR description says "10 trigger phrases" but the fixture has 8. The PR body says "Adds tests/fixtures/trigger_phrases.txt (10 phrases)" and "10 canonical trigger phrases added", but tests/fixtures/trigger_phrases.txt has 8 lines (the round-1 review flagged 2 ambiguous phrases for removal and the author dropped them — correctly). Update the PR body so the merge commit description is accurate; this is the artifact reviewers and historians will look at.

Verification

$ uv run --no-project pytest tests/ -v
============================== 19 passed in 0.02s ==============================

All 19 tests pass at 0997ec9. The two new parity tests (120 s, Confirmation gate) work as designed. The third new test (test_build_inside_trap_scope) catches the recce-analyze.md variant of ISSUE 1 (empirically verified) but has the gap described above for AGENTS.md.

What I could not verify

• End-to-end execution against a real dbt project — whether <chosen build command> produces an unambiguous bash error if an agent fails to substitute (theory predicts loud failure via <chosen input redirection on a missing file; not exercised here).
• Whether Codex's actual fence-execution semantics match the documented per-fence-subprocess assumption.

What I looked for and did not find

• New parity drift between recce-analyze.md and AGENTS.md (the fix is symmetric: same explanatory paragraph, same fence structure, same placeholder line, same comment commands).
• Stale /recce-pr references in non-deprecation contexts (grep confirms remaining references at recce-check.md:25, recce-pr.md:11, recce-pr.md:25, recce-setup.md:7,168, SKILL.md:49-50 are all in deprecation-notice or "Legacy commands" sections — intentional).
• New cleanup-function regressions; both files retain the conflict-on-pop user-visible warning that the prose claims.

🤖 Generated with Claude Code via /recce-dev:claude-code-review

Yup, I'm setting up Codex to check all of them now.

@wcchang1115 thanks for the thorough review — addressed all blockers + issues + 2 of 3 notes in fb7146c.

BLOCKERs

1. docs_generate corrupts target-base/ — Fixed in both commands/recce-analyze.md and AGENTS.md. Both docs_generate and full_build now run from the base branch via the safe stash dance (see ISSUE 1 below). The docs_generate path no longer compiles target-branch SQL into base manifests.

2. AGENTS.md missing prereq + branch detection — Added Step 1 — Prerequisites (dbt_project.yml / dbt --version / recce --version) and Step 2 — Branch detection to AGENTS.md, mirroring recce-analyze.md. Trigger phrases can no longer skip straight to recce check-base on a fresh project. (You also flagged this might not warrant blocker status — addressed anyway since the prereq layer was genuinely absent.)

ISSUEs

1. Unsafe stash dance — Replaced git stash; checkout; build; checkout; pop with the named-stash + EXIT-trap pattern in both files: git stash push --include-untracked -m "recce-analyze-<ts>", capture stash-id, pop only by exact message, trap cleanup EXIT always returns the user to the target branch and surfaces unpopped stashes instead of swallowing them.

2. "Byte-equivalent parity" was false — Reconciled all four divergences:

   • Full staleness warning sentence (verbatim) now appears in AGENTS.md too.
   • Dropped fictional ${CODEX_PLUGIN_ROOT} — both files use ${CLAUDE_PLUGIN_ROOT} and AGENTS.md instructs Codex to substitute the literal path (since Codex doesn't export the var).
   • MCP tool names use mcp__recce__* prefix in both files (matches the actual server-registered names).
   • Added Error Recovery section to AGENTS.md.

   Added 4 parity tests in test_merged_bootstrap.py so any future drift fails CI:

   • test_stale_warning_parity_claude_codex
   • test_mcp_tool_names_parity
   • test_no_codex_plugin_root_in_agents_md
   • test_safe_stash_dance_in_both_paths

3. Trigger-phrase auto-execute too aggressive — Two fixes layered:

   • Dropped the two most ambiguous phrases ("review my changes", "show me what broke"). Trigger set is now 8 phrases. Fixture + count test updated.
   • Added an explicit Confirmation gate inside Step 3, REQUIRED before any git checkout or dbt build. Both files now print a one-line summary (base/target/expected runtime) and require y/N confirmation regardless of how the flow was triggered. SKILL.md now says trigger phrases propose /recce-analyze rather than execute it silently.

4. Vacuous AC-2 / AC-3 tests — Tightened:

   • AC-2: "120" in text → anchored regex re.search(r"\b120\s*s\b", text).
   • AC-3: "stale" in text.lower() → exact substring match on the full warning sentence "⚠️ Base artifacts are stale. Refreshing with dbt docs generate…".

5. SKILL.md SessionStart routed to deprecated /recce-setup — Swapped to /recce-analyze. Same fix applied to commands/recce-pr.md and commands/recce-check.md STATUS=NOT_RUNNING branches (NOTE 3 below).

NOTEs

1. recce check-base no in-procedure fallback — Added a one-line fallback in both files: on non-zero exit / non-JSON / unknown recommendation, recommend pip install -U recce and fall back to full_build via the safe stash dance. Removes the recce#1353 merge-order coupling.

2. Deprecation banners lack timeline / telemetry — Acknowledged but not addressed in this PR (out of scope; tracked separately). Will file a follow-up.

3. recce-pr.md:24 routed to deprecated /recce-setup — Fixed (see ISSUE 5).

Verification

$ uv run pytest tests/ -v
============================== 16 passed in 0.03s ==============================

10 → 16 tests; the 6 new tests are the parity guards above plus the prereq + Error Recovery checks for AGENTS.md. All passing.

Ready for re-review.

@wcchang1115 Round-2 findings addressed in 0997ec95. All 19 tests pass at the new HEAD (16 prior + 3 new).

ISSUE 1 — build outside trap scope (FIXED)

You're right — splitting the build into inline-prose bullets after the trap-protected fence meant the trap fired before the build could run, leaving target-branch SQL in target-base/. Fixed in both files by moving the build inside the same fence:

git checkout <base-branch>

# Run ONE based on Step 3 recommendation (substitute below):
# - docs_generate: dbt docs generate --target-path target-base
# - full_build:    dbt build         --target-path target-base
<chosen build command>
# trap fires on script exit → returns to $TARGET_BRANCH and pops stash.

The <chosen build command> placeholder is now an explicit line that the agent must replace — not just a comment. Both recce-analyze.md and AGENTS.md got the same treatment plus a leading paragraph documenting why (per-fence subprocess isolation + trap timing) so a future edit doesn't quietly split it back out.

ISSUE 2 — /recce-pr recommendation in SKILL.md (FIXED)

SKILL.md:100-109 swapped to /recce-analyze with bullets aligned to the merged 4-section output (Impact Summary / Lineage Changes / Schema Changes / Row Count Changes). Same defect class as round-1 ISSUE 5 — agreed it should have been caught alongside the SessionStart fix.

NOTE 1 + NOTE 2 — test coverage gaps (FIXED)

Three tests added in tests/test_merged_bootstrap.py:

Sanity check: I mentally ran test_build_inside_trap_scope against the pre-fix state — it would have asserted on the recce-analyze.md fence (no dbt command inside, no placeholder), exactly the regression you described.

Verification

$ uv run --no-project pytest tests/ -v
============================== 19 passed in 0.02s ==============================

Asking for one more pass when convenient.

@wcchang1115 Round-3 finding addressed in 412b252c.

ISSUE 1 — test_build_inside_trap_scope AGENTS.md variant gap (FIXED)

You're right — the round-2 substring check matched comment text, so the AGENTS.md "comments-inside-fence-with-no-executable-line" variant of round-2 ISSUE 1 would pass through CI green. The commit message overshipped the test's actual coverage.

Applied your suggested patch verbatim — added _exec_lines() helper that drops blank and comment-only lines before the substring check:

def _exec_lines(body: str) -> str:
    return "\n".join(
        line for line in body.splitlines()
        if line.strip() and not line.strip().startswith("#")
    )

test_build_inside_trap_scope now runs the trap-fence body through _exec_lines() before asserting on dbt docs generate / dbt build / <chosen build command> — # dbt docs generate ... comments no longer satisfy.

Empirical proof — round-3 recommended regression test (ADDED)

Took the optional-but-recommended step: added test_build_inside_trap_scope_catches_pre_fix_agents_variant that constructs the exact pre-fix AGENTS.md fence body inline (the # docs_generate: dbt docs generate ... comment-only form) and asserts the filter rejects it. Confirms the tightening covers the variant the round-2 test let through.

NOTE 1 — PR body counts (FIXED)

Updated the PR body for merge-commit-description accuracy:

• "10 canonical trigger phrases" → "8 canonical trigger phrases (round-1 review dropped 2 ambiguous)"
• "(10 phrases)" → "(8 phrases)"
• "10/10 tests pass" → "20/20 tests pass"
• "test_merged_bootstrap.py 6 tests" → "test_merged_bootstrap.py 16 tests"

Verification

$ uv run --no-project pytest tests/ -v
============================== 20 passed in 0.04s ==============================

20/20 pass at 412b252c — 19 prior + the new empirical-proof test. The tightened assertion still passes on the current source files because both contain the explicit <chosen build command> placeholder.